Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

style: Use f-strings wherever possible #600

Merged
merged 1 commit into from
Oct 6, 2023
Merged

style: Use f-strings wherever possible #600

merged 1 commit into from
Oct 6, 2023

Conversation

DimitriPapadopoulos
Copy link
Contributor

They're faster.

adrienverge
adrienverge reviewed Oct 2, 2023
View reviewed changes
Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for taking the time!

Have you changed them one by one and checked they all make sense? Or did you use a script to find and replace them? (in some rare cases like %02d, %.2f or %-10s, f-string cannot be used as is)

tests/test_spec_examples.py Show resolved Hide resolved
yamllint/config.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 2, 2023 •
edited
Loading

Coverage Status

coverage: 99.391% (-0.002%) from 99.393% when pulling cb7e19a on DimitriPapadopoulos:f-strings into 816d575 on adrienverge:master.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 2, 2023 •
edited
Loading

Have you changed them one by one and checked they all make sense? Or did you use a script to find and replace them? (in some rare cases like %02d, %.2f or %-10s, f-string cannot be used as is)

There are not such cases as far as I can see. Yet, it possible to write f{variable!02d}f{variable:02d} or f{variable!.2f}f{variable:.2f}, isn't it?

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review October 2, 2023 20:35
@adrienverge
Copy link
Owner

There are not such cases as far as I can see. Yet, it possible to write f{variable!02d} or f{variable!.2f}, isn't it?

Sure it is! I just wanted to make sure you (or your custom script?) looked for them, if any :)

I still see a few %s and %d in some files (namely yamllint/linter.py, yamllint/rules/line_length.py, yamllint/rules/indentation.py and yamllint/rules/octal_values.py). Could you check them?

@DimitriPapadopoulos
Copy link
Contributor Author

As for the tool I use, it's pyupgrade. But I often end up applying changes manually, because:

  1. You cannot limit pyupgrade to a single class of changes. I prefer an individual commit per class of changes.
  2. It's not perfect: it often leaves behind some cases of % string interpolation.

@DimitriPapadopoulos
Copy link
Contributor Author

I have left an instance of % string interpolation in yamllint/rules/line_length.py because I find the f-string interpolation is less readable – unless of course I use additional variables to store the result of complex expressions.

adrienverge
adrienverge approved these changes Oct 6, 2023
View reviewed changes
Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left an instance of % string interpolation in yamllint/rules/line_length.py

I had spotted this one too and I agree it's better readable like this 👍

I allowed myself to force-push to your branch with a slight rework of modified strings:

--- a/yamllint/config.py
+++ b/yamllint/config.py
@@ -179,8 +179,8 @@ def validate_rule_conf(rule, conf):
                 continue
             if optkey not in options:
                 raise YamlLintConfigError(
-                    f'invalid config: unknown option "{optkey}" '
-                    f'for rule "{rule.ID}"')
+                    f'invalid config: unknown option "{optkey}" for rule '
+                    f'"{rule.ID}"')
             # Example: CONF = {option: (bool, 'mixed')}
             #          → {option: true}         → {option: mixed}
             if isinstance(options[optkey], tuple):
@@ -216,8 +216,8 @@ def validate_rule_conf(rule, conf):
                 raise YamlLintConfigError(f'invalid config: {rule.ID}: {res}')
     else:
         raise YamlLintConfigError(
-            f'invalid config: rule "{rule.ID}": should be '
-            f'either "enable", "disable" or a dict')
+            f'invalid config: rule "{rule.ID}": should be either "enable", '
+            f'"disable" or a dict')
 
     return conf
 
--- a/yamllint/rules/indentation.py
+++ b/yamllint/rules/indentation.py
@@ -342,11 +342,11 @@ def _check(conf, token, prev, next, nextnext, context):
 
         if found_indentation != expected:
             if expected < 0:
-                message = f'wrong indentation: expected ' \
-                          f'at least {found_indentation + 1}'
+                message = f'wrong indentation: expected at least ' \
+                          f'{found_indentation + 1}'
             else:
-                message = f'wrong indentation: expected {expected} ' \
-                          f'but found {found_indentation}'
+                message = f'wrong indentation: expected {expected} but ' \
+                          f'found {found_indentation}'
             yield LintProblem(token.start_mark.line + 1,
                               found_indentation + 1, message)

Thank you Dimitri, let's merge!

@adrienverge adrienverge merged commit f63e56f into adrienverge:master Oct 6, 2023
5 of 6 checks passed
@DimitriPapadopoulos DimitriPapadopoulos deleted the f-strings branch October 6, 2023 12:01
This was referenced Dec 31, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrienverge adrienverge adrienverge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DimitriPapadopoulos @coveralls @adrienverge